Skip to content

fix(downloader): distinguish read from write failures and retry transient ones#10985

Merged
mudler merged 1 commit into
masterfrom
fix/download-retry-and-errors
Jul 20, 2026
Merged

fix(downloader): distinguish read from write failures and retry transient ones#10985
mudler merged 1 commit into
masterfrom
fix/download-retry-and-errors

Conversation

@localai-bot

Copy link
Copy Markdown
Collaborator

Fixes #10982.

1. A read error was reported as a write failure

io.Copy returns read and write errors indistinguishably, and uri.go labelled every one of them failed to write file.

In the incident that surfaced this, stream error: stream ID 23; CANCEL; received from peer — an http2.StreamError, which can only come from source.Read — was reported as a write failure. That sent the investigation after filesystem permissions and CIFS write semantics for an hour. The disk was fine the whole time.

The copy source is now wrapped in a small recorder that remembers the last non-EOF read error; comparing it against the error io.Copy returned says which side broke. A read failure names the URL it was reading and the file it was writing into; a write failure now names the .partial, which is the file actually being written, rather than the final blob path.

2. One transient error aborted a whole multi-file materialization

DownloadFilesWithContext returned on the first task error with no retry. Stream ID 23 means roughly the twelfth request on that connection: eleven files had already downloaded successfully, and all of it was discarded.

The .partial resume machinery already existed (uri.go computes startPos from the existing partial's size) but nothing ever retried, so it was unreachable in practice.

Transient failures are now retried within the plan, resuming from the partial.

Failure Retry Why
Transport error from downloadClient.Do yes reset/refused/TLS hiccup; nothing about the URL is wrong
Mid-stream read failure (HTTP/2 CANCEL, unexpected EOF, stall guard) yes bytes already on disk are valid, resume from .partial
5xx, 429 yes describes the server's current state, not the request
Resume request answered non-206 yes that site discards the partial, so the retry starts clean and self-heals
Other 4xx (404, auth) no settled; retrying only delays the real error
SHA mismatch no deterministic, and the partial is deleted — a retry would re-fetch GBs for nothing
Local write failure (ENOSPC, perms, broken mount) no same bytes to the same broken target
Caller cancellation / ErrUserCancelled no IsRetryable short-circuits on ctx.Err() != nil before classification

Bounded at 3 attempts with 2s/4s backoff, tunable via DownloadRetryAttempts / DownloadRetryBaseDelay in the style of the existing DownloadStallTimeout. The small budget is deliberate: resume makes a retry cheap in bytes, but a tight loop against a genuinely broken remote on multi-GB files is its own hazard. Backoff waits on ctx.Done().

No existing retry helper was found in pkg/ or core/, so this is new.

Scope

Both fixes are storage-backend independent and reproduce on local disk with a flaky network — no CIFS and no concurrency required. They were surfaced by #10981 but are not caused by it.

Plan callers (pkg/modelartifacts/materializer.go, core/gallery/models.go) inherit the retry with no signature change and are untouched.

Repo-wide grep including tests/e2e/ found nothing pinning failed to write file, invalid status code, resume request for, or plan-abort behaviour.

Tests

Ginkgo specs in pkg/downloader/retry_test.go, driven by a flaky range server that aborts the connection mid-body:

  • a read-side failure is not labelled a write failure, and names its source
  • a write-side failure names the .partial
  • two transient failures are retried, and the retries carry bytes=N- Range headers — proving resume rather than restart — with correct final content
  • a 404 is attempted exactly once
  • a cancelled caller is attempted exactly once

Red before implementation:

Summarizing 2 Failures:
  [FAIL] download failure diagnostics [It] does not report a failed response read as a write failure
  [FAIL] DownloadFilesWithContext retries [It] retries a transient stream failure and resumes from the partial
      failed to write file "/tmp/ginkgo4024210465/model.gguf": unexpected EOF
Ran 49 of 49 Specs -- 47 Passed | 2 Failed

That first failure message is the bug verbatim: a mid-stream read abort labelled as a write failure.

Verification: go build ./core/... ./pkg/... clean; go test ./pkg/downloader/ ./pkg/xio/ ok; make lint 0 issues.


🤖 Generated with Claude Code

…ient ones

Two independent defects in the download path, both surfaced by the same
incident (#10982).

A failed `io.Copy` was always reported as "failed to write file", because
`io.Copy` folds read and write errors into a single return value. A
peer-cancelled HTTP/2 stream therefore presented as a filesystem failure and
sent an investigation after mount permissions while the disk was healthy. The
source is now wrapped in a recorder so the error names the side that actually
broke, and a write failure names the `.partial` it was writing rather than the
final blob path.

The plan runner returned on the first task error with no retry, so one
transient stream cancel discarded every file already downloaded in a
multi-file materialization. The `.partial` resume machinery already existed
but was unreachable because nothing made a second attempt. Transient failures
(dropped transport, mid-stream read failure, stall, 5xx, 429) are now retried
with bounded exponential backoff and resume from the partial; permanent ones
(4xx, checksum mismatch, local write failure, caller cancellation) fail
immediately.

Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
@mudler
mudler merged commit 0d9d07d into master Jul 20, 2026
21 of 22 checks passed
@mudler
mudler deleted the fix/download-retry-and-errors branch July 20, 2026 17:56
mudler added a commit that referenced this pull request Jul 21, 2026
…#11017)

backendLoader logged "BackendLoader starting" at INFO as its very first
statement, unconditionally. That reads as "a model is being loaded", but
backendLoader is not only a load path: in distributed mode Load()
deliberately bypasses the local cache and calls backendLoader on every
inference request so SmartRouter can re-pick a replica per request. The
model is already resident, no process is spawned, and nothing is loaded,
yet the banner fires at request rate.

On a live cluster this produced ~5 "BackendLoader starting" lines per
second for a single embedding model, sustained, starting 22 seconds
after the load had already completed. The model was state=loaded with
in_flight=0 and exactly one backend process on the worker. It looked
exactly like a retry storm and cost real debugging time during an
unrelated production investigation. The adjacent "effective runtime
tuning" banner, documented as "logged once per load", had the same
problem for the same reason.

Emit both banners at INFO only when the model is not already resident,
and keep the per-call trace at DEBUG for anyone following the routing
path. isResident is a plain store lookup with no health probe and no
eviction, so it is safe on the per-request hot path (unlike
checkIsLoaded, which probes and can evict).

Same class of defect as #10985: a log line that sends the reader after
the wrong thing.


Assisted-by: Claude Code:claude-opus-4-8[1m] [Read] [Edit] [Bash]

Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Co-authored-by: Ettore Di Giacinto <mudler@localai.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

download: HTTP/2 stream error reported as a write failure, and one transient error aborts a whole multi-file materialization

2 participants